feat(mergeProps): utility to merge component props and global config #645
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough本次变更新增了一个 Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant mergeProps
Caller->>mergeProps: mergeProps(defaultProps, config, props)
mergeProps-->>Caller: 返回合并后的对象（props > config > defaultProps）
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 分钟 Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/mergeProps.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. src/index.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. tests/mergeProps.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/mergeProps.ts (1)
12-18: 对象检测逻辑与递归调用混淆
mergeClassName内部用typeof x === 'object'判断是否进入mergeClassNames，但这同时把null、array、Date等也算进去。考虑改用isPlainObject判断可避免误判；或直接信赖父函数的classNames键已明确限定。tests/mergeProps.test.ts (1)
34-55: 建议增加“三对象合并”用例当前仅验证两对象场景。由于实现对三个及以上对象采用递归策略，最好补充类似：
expect( mergeProps( { className: 'a' }, { className: 'b' }, { className: 'c' }, ), ).toEqual({ className: 'a b c' });可防止未来修改递归逻辑导致回归。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/index.ts(1 hunks)src/mergeProps.ts(1 hunks)tests/mergeProps.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/mergeProps.test.ts (1)
src/mergeProps.ts (1)
mergeProps(20-38)
🔇 Additional comments (1)
src/index.ts (1)
3-3: 公开导出 👍将
mergeProps暴露在入口处符合本次 PR 目标，方便外部直接引用。
package.json
Outdated
| "classnames": "^2.5.1", | ||
| "is-plain-object": "^5.0.0", | ||
| "react-is": "^18.2.0" |
There was a problem hiding this comment.
is-plain-object 需要默认导入，否则运行时会是 undefined
is-plain-object 采用 CommonJS 导出方式（module.exports = fn），在 ESM/TS 中通常通过默认导入获得函数实例。当前包以命名导入形式引用，将导致 isPlainObject 为 undefined，进而在 mergeProps 中触发运行时异常。
请在 src/mergeProps.ts 内改为默认导入，或在此处添加 @types/is-plain-object 并更新导入方式。参考补丁见下方代码评论。
🤖 Prompt for AI Agents
In package.json around lines 41 to 43, the dependency "is-plain-object" is
listed but the import usage in the code is incorrect due to its CommonJS export
style. To fix this, ensure that in src/mergeProps.ts the import of
"is-plain-object" is changed to a default import rather than a named import, or
alternatively add the @types/is-plain-object package and update the import
accordingly to avoid runtime undefined errors.
src/mergeProps.ts
Outdated
| import classNames from 'classnames'; | ||
| import { isPlainObject } from 'is-plain-object'; |
There was a problem hiding this comment.
修正 is-plain-object 的导入方式
-import { isPlainObject } from 'is-plain-object';
+import isPlainObject from 'is-plain-object';如前所述，is-plain-object 采用默认导出，命名导入将拿到 undefined。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import classNames from 'classnames'; | |
| import { isPlainObject } from 'is-plain-object'; | |
| import classNames from 'classnames'; | |
| import isPlainObject from 'is-plain-object'; |
🤖 Prompt for AI Agents
In src/mergeProps.ts at lines 1 to 2, the import of isPlainObject from
'is-plain-object' is incorrect because the package uses a default export. Change
the import statement to use a default import instead of a named import to
correctly import the function.
src/mergeProps.ts
Outdated
| Object.keys(list[1]).forEach(key => { | ||
| if (key === 'className') { | ||
| result[key] = classNames(result[key], list[1][key]); | ||
| } else if (key === 'classNames') { | ||
| result[key] = mergeClassNames(result[key], list[1][key]); | ||
| } else if (isPlainObject(list[1][key])) { | ||
| result[key] = mergeProps(result[key], list[1][key]); | ||
| } else { |
There was a problem hiding this comment.
递归合并时基对象可能为 undefined，需降级为空对象
- } else if (isPlainObject(list[1][key])) {
- result[key] = mergeProps(result[key], list[1][key]);
+ } else if (isPlainObject(list[1][key])) {
+ result[key] = mergeProps(result[key] ?? {}, list[1][key]);若 result[key] 尚未定义，直接展开会抛错。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Object.keys(list[1]).forEach(key => { | |
| if (key === 'className') { | |
| result[key] = classNames(result[key], list[1][key]); | |
| } else if (key === 'classNames') { | |
| result[key] = mergeClassNames(result[key], list[1][key]); | |
| } else if (isPlainObject(list[1][key])) { | |
| result[key] = mergeProps(result[key], list[1][key]); | |
| } else { | |
| Object.keys(list[1]).forEach(key => { | |
| if (key === 'className') { | |
| result[key] = classNames(result[key], list[1][key]); | |
| } else if (key === 'classNames') { | |
| result[key] = mergeClassNames(result[key], list[1][key]); | |
| } else if (isPlainObject(list[1][key])) { | |
| result[key] = mergeProps(result[key] ?? {}, list[1][key]); | |
| } else { | |
| // … | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/mergeProps.ts around lines 26 to 33, when recursively merging properties,
the base object result[key] might be undefined, causing errors when passed to
mergeProps. To fix this, ensure result[key] is defaulted to an empty object if
undefined before passing it to mergeProps, preventing runtime exceptions during
recursive merging.
src/mergeProps.ts
Outdated
| function mergeClassNames<T>(classNamesA: T, classNamesB: T): T { | ||
| const result = { ...classNamesA }; | ||
| Object.keys(classNamesB).forEach(key => { | ||
| result[key] = mergeClassName(classNamesA[key], classNamesB[key]); | ||
| }); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
mergeClassNames 未对 undefined 做保护，传播时会崩溃
当 result[key] 初始为 undefined（最常见场景是只有第二个对象携带 classNames）时，{ ...classNamesA } 会抛出 TypeError。
-function mergeClassNames<T>(classNamesA: T, classNamesB: T): T {
- const result = { ...classNamesA };
- Object.keys(classNamesB).forEach(key => {
+function mergeClassNames<T extends Record<string, any>>(
+ classNamesA: T = {} as T,
+ classNamesB: T = {} as T,
+): T {
+ const result: Record<string, any> = { ...classNamesA };
+ Object.keys(classNamesB || {}).forEach(key => {
result[key] = mergeClassName(classNamesA[key], classNamesB[key]);
});
- return result;
+ return result as T;
}这样可兼容首参缺失场景，并补全类型。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function mergeClassNames<T>(classNamesA: T, classNamesB: T): T { | |
| const result = { ...classNamesA }; | |
| Object.keys(classNamesB).forEach(key => { | |
| result[key] = mergeClassName(classNamesA[key], classNamesB[key]); | |
| }); | |
| return result; | |
| } | |
| function mergeClassNames<T extends Record<string, any>>( | |
| classNamesA: T = {} as T, | |
| classNamesB: T = {} as T, | |
| ): T { | |
| const result: Record<string, any> = { ...classNamesA }; | |
| Object.keys(classNamesB || {}).forEach(key => { | |
| result[key] = mergeClassName(classNamesA[key], classNamesB[key]); | |
| }); | |
| return result as T; | |
| } |
🤖 Prompt for AI Agents
In src/mergeProps.ts around lines 4 to 10, the mergeClassNames function does not
handle undefined inputs, causing a TypeError when classNamesA is undefined. To
fix this, add checks to ensure classNamesA and classNamesB default to empty
objects if undefined before spreading or accessing their keys. This will prevent
crashes when the first argument is missing and maintain proper typing.
src/mergeProps.ts
Outdated
| if (list.length > 2) { | ||
| return mergeProps(list[0], mergeProps(...list.slice(1))); | ||
| } |
There was a problem hiding this comment.
这个是递归操作么？试试看用 ai 能不能改成 尾调用方式 （乱说的，不知道有没有用
src/mergeProps.ts
Outdated
|
|
||
| export default function mergeProps<T>(...list: T[]): T { | ||
| if (list.length > 2) { | ||
| return mergeProps(list[0], mergeProps(...list.slice(1))); |
There was a problem hiding this comment.
这个对 antd 没什么用。className 部分 antd 用的语义化 classNames 有自己的 hooks 封装。其他属性 merge 推荐直接用 {...A,...B} ，浏览器原生性能更好。用递归 + 解构 会产生大量的临时数组，此外多个 if else + isPlainObject 都是浪费性能的。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #645 +/- ##
==========================================
+ Coverage 90.19% 90.21% +0.02%
==========================================
Files 36 37 +1
Lines 877 879 +2
Branches 285 291 +6
==========================================
+ Hits 791 793 +2
Misses 84 84
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(1 hunks)src/mergeProps.ts(1 hunks)tests/mergeProps.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/mergeProps.test.ts
- src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test / react component workflow
| export default function mergeProps<A, B, C>( | ||
| defaultProps: A, | ||
| config: B, | ||
| props: C, | ||
| ): A & B & C { | ||
| return { ...defaultProps, ...config, ...props }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
质疑这个抽象层的必要性
这个函数本质上就是对 {...defaultProps, ...config, ...props} 的简单封装。正如之前 zombieJ 提到的，直接使用原生展开语法性能更好，而且代码同样简洁易读。
考虑以下几点：
- 性能开销：增加了额外的函数调用层
- 过度抽象：对于如此简单的操作，封装可能带来的价值有限
- 直观性：
{...a, ...b, ...c}的意图在代码中更加直观明确
建议重新评估这个工具函数是否真的能带来足够的价值来justify其存在。
🤖 Prompt for AI Agents
In src/mergeProps.ts lines 1 to 7, the mergeProps function is a simple wrapper
around the object spread syntax, which adds unnecessary abstraction and function
call overhead. To fix this, remove the mergeProps function entirely and replace
its usage with direct object spread expressions like {...defaultProps,
...config, ...props} in the codebase to improve performance and maintain code
clarity.
用通用的语法来取代目前组件中各种手动合并 props 的代码，减少代码量，更易读。
Summary by CodeRabbit
mergeProps工具函数，用于合并多个对象属性，后传入的对象属性会覆盖前面的属性。mergeProps函数的单元测试，验证对象属性合并的正确性。